Repository navigation
Conversation
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
LeftHandCold
left a comment
LeftHandCold
left a comment
There was a problem hiding this comment.
代码复核：建议通过，未发现需要 Request Changes 的确定性问题。审查 HEAD：af00ad672a186786720edc96e403497306e2fdd3。
设计判断：局部无状态算术优化，不改变函数接口、类型或时区职责。
逐项核对：
- 仅有效的非负 DATETIME 范围走秒/分/时/日的微秒取余；该表示从公历午夜起算，因此固定单位边界与原日历分解一致。
- ZeroDatetime、负原始值、越界原始值和月/季/周/年仍走原处理；没有把负数截断或超范围兼容行为偷偷改掉。
- DATE 的子日单位拒绝仍在调用层；TIMESTAMP 的时区、夏令时折叠和回转逻辑仍由原调用层负责，不是直接对 UTC 值按当地日取模。
- 新测试的日历 oracle 独立于新算法，覆盖闰年、世纪、1924 边界、年 1/9999、微秒和 NULL/错误顺序。未引入分配、锁或长期状态。
验证边界：已核对当前 HEAD 的成功 CI 40023230517，并完成 diff --check。本次为代码与既有证据复核，没有重新运行 TSBS；PR 中的收益数字仍限定于作者测量场景。
当前认证账号也是 PR 作者，故以 COMMENT 记录建议通过；这不是 GitHub 正式 Approval。
|
mo-bot review · Commit: Review completed. Read the assessment. |
XuPeng-SH
left a comment
XuPeng-SH
left a comment
There was a problem hiding this comment.
mo-bot review · mo-bot-local · review
Commit: af00ad672a186786720edc96e403497306e2fdd3
Assessment: approve
Reviewed the complete three-file delta and affected temporal types, SQL overloads, and vector callers. The guarded arithmetic preserves calendar truncation for valid DATETIME values while retaining raw-value fallback, DATE restrictions, NULL/error ordering, and TIMESTAMP timezone/DST handling. The optimization belongs in the existing owner and introduces no allocation or retained state. No actionable findings remain.
Model: gpt-6-astra / xhigh
Session: 01a13dcf-1586-7a01-9000-d45e20995cf8 · slot 1 · generation 1
Validation:
-
Reviewed Gregorian-oracle tests, epoch/year-9999 boundaries, leap/century cases, negative/year-65537 fallback controls, vector NULL/zero/error behavior, benchmark quality, and existing DATE/DST and SQL regression coverage.
-
Reused frozen exact-head successful UT, SCA, shared build, coverage, proxy BVT, and pessimistic multi-CN BVT statuses. Repository UT selection includes pkg/sql/plan/function; skipped jobs were not counted as passes.
-
Local static audit passed revision/base verification, source cleanliness, complete patch scope, git diff --check, and gofmt. Receipt: checks/1792809017213820158-4.json; retained log: review-static.log.
-
Performance tracing confirms removal of calendar decomposition for fixed units without new allocations. Calendar/raw-value fallback adds bounded guard/dispatch work; paths not using DATE_TRUNC are unchanged.
-
Saved review-assessment.json and review-evidence.json in the assigned diagnostic directory.
Limitations:
-
No local Go tests or benchmarks were rerun. local_validation is empty; frozen CI supplies job outcomes rather than per-test logs or author execution receipts.
-
Author-reported TSBS medians improved from 302.1 to 250.0 ms (Q1), 340.2 to 278.3 ms (Q2), and 352.6 to 287.1 ms (Q5). Raw measurements were unavailable; these isolated-patch results were not independently reproduced and do not establish combined-patch gains.
-
The linked issue #31003 body was absent; its contract was assessed through the frozen PR description and discussion.
|
Queued — the merge queue status continues in this comment ↓. |
Merge Queue Status
This pull request spent 1 hour 8 minutes 39 seconds in the queue, with no time running CI. ReasonPull request #31004 has been dequeued Pull request from fork cannot be queued. This pull request comes from a fork, and Mergify needs the author's permission to update its branch.
HintYou should look at the reason for the failure and decide if the pull request needs to be fixed or if you want to requeue it. Waiting for any of
All conditions
Tick the box to put this pull request back in the merge queue (same as
|
What type of PR is this?
Which issue(s) this PR fixes:
Fixes #31003.
What this PR does / why we need it:
DATE_TRUNCdecomposes and rebuilds the calendar for every row, including fixed second/minute/hour/day boundaries. For valid DATETIME values, clear the remainder of the corresponding microsecond period directly. Keep the original fallback for raw invalid values and calendar units. Vector NULL/error handling, DATE validation and TIMESTAMP timezone/DST handling are unchanged. The production change is 15 lines and adds no allocation or retained state.On the original TSBS CPU-only dataset (8.64 million measurement rows, 1,000 tags), alternating A/B/B/A with ten measured samples per version/query gives:
All five original queries retain complete source-checked outputs. Q3/Q4 controls do not use DATE_TRUNC; their median wall changes are +1.36 ms/+0.59% and CPU changes +0.83%/+0.17%. The whole engine/client/validator shares four CPU cores and a 12 GiB, no-swap cgroup. Whole-block memory peaks are 1.478–1.532 GiB with zero OOM/limit events. Profiles were collected separately from timing. This A/B isolates only this patch on
b78794175e65; Top-1 and other candidate optimizations are not combined.Validation:
af00ad672ais rebased onto verified main1651eec948. All three changed source/test files are byte-identical across the rebase; datetime callers/types/vector/registration are unchanged. Full owning-package tests, gofmt/diff checks, scoped vet, configured incremental golangci-lint and native-provenance checks all pass on this delivery head.This is a draft for review. Measurements establish the stated CPU-only improvement; DevOps/IoT and broader benchmark optimization remain separate work.